Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java: Big Decimal DOS #6730

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

tonghuaroot
Copy link

Directly incorporating user input into an BigDecimal Operation Function without validating the input
can facilitate DOS attacks. In these attacks, the server
will consume a lot of computing resources, A typical scenario is that the CPU usage rises to close to 100%.
This issue often occurs in scenarios that require scientific computing, such as e-commerce platforms and electronic payments.

override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }

override predicate isSink(DataFlow::Node sink) {
exists(Method method, MethodAccess call |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered adding other methods of the BigDecimal class that could be equally problematic, e.g. divide, multiply or pow?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @atorralba for helping review this part of the code.
Yes, I will do more tests and add these methods!

Copy link
Contributor

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a member of this project, so feel free to consider these comments only as suggestion, but maybe they are helpful nontheless.



<overview>
<p>Directly incorporating user input into an BigDecimal Operation Function without validating the input
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use the term "method" instead of "function" since that is the commonly used term in the Java context.
(And also a few lines below the term "method" is used as well)

<recommendation>

<p>To guard against BigDecimal DOS attacks, you should avoid putting user-provided input
directly into a BigDecimal Method(like: add(), subtract()). Instead, In some e-commerce payment scenarios,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
directly into a BigDecimal Method(like: add(), subtract()). Instead, In some e-commerce payment scenarios,
directly into a BigDecimal method (like: add(), subtract()). Instead, in some e-commerce payment scenarios,

Maybe it would also be worth formatting these methods as code using <code>...</code>?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Marcono1234 help review, I will to do some modify about this query rule's help file.

@@ -0,0 +1,38 @@
/**
* @id java/BigDecimalDOS
* @name Java-BigDecimal-DOS-Vulnerability
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name does not have to use hyphens, see query metadata style guide

@ResponseBody
// GOOD:
public Long demo02(@RequestParam(name = "num") String num) {
if (num.length() > 33 || num.matches("(?i)e")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be:

Suggested change
if (num.length() > 33 || num.matches("(?i)e")) {
if (num.length() > 33 || num.matches("(?i).*e.*")) {

(or similar)

}
Long startTime = System.currentTimeMillis();
BigDecimal num1 = new BigDecimal(0.005);
System.out.println(num1.add(num));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no BigDecimal.add(String); you would probably have to convert num it to a BigDecimal first

@atorralba
Copy link
Contributor

Note that tests are failing:

FAILED: java/ql/test/experimental/query-tests/security/BigDecimalDOS/BigDecimalDOS.qlref
FAILED: java/ql/test/experimental/query-tests/security/BigDecimalDOS/DB-CHECK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants